[Nexthop][fboss2-dev] Add fboss2-dev delete copp subcommands (cpu-queue, reason) - #1466
[Nexthop][fboss2-dev] Add fboss2-dev delete copp subcommands (cpu-queue, reason)#1466vybhav-nexthop wants to merge 3 commits into
Conversation
|
This pull request has been imported. If you are a Meta employee, you can view this in D114872682. (Because this pull request was imported automatically, there will not be any future comments.) |
Delete-side counterparts for the copp config commands:
delete copp cpu-queue <id> - remove the whole sw.cpuQueues[]
entry; refused while rxReason mappings (the ordered list or the
deprecated rxReasonToCPUQueue map, which the agent still reads as
a fallback) or matchToAction actions (send-to-queue,
user-defined-trap) still reference the queue id, naming each
referencing entry
delete copp reason <reason-name> - remove the
rxReasonToQueueOrderedList entry
Per-attribute deletes (cpu-queue <id> name, cpu-queue <id> rate-limit)
are left for a follow-up.
Queue-id parsing/lookup and reason-name parsing move out of
CmdConfigCopp.cpp into the copp_cpu_queue and copp_reason namespaces in
a new CoppUtils.{h,cpp} so the config and delete trees share one
implementation. Both handlers save with ConfigActionLevel::HITLESS,
matching the config-side handlers.
This also introduces the `delete copp` parent command node, which does
not exist upstream yet (facebook#1379 adds it too, for the cpu-traffic-policy
delete; whichever lands second is a trivial merge).
e99d556 to
1e2e79f
Compare
|
@vybhav-nexthop has updated the pull request. You must reimport the pull request before landing. |
|
@vybhav-nexthop has updated the pull request. You must reimport the pull request before landing. |
joseph5wu
left a comment
There was a problem hiding this comment.
The main comment for this PR to fix is to remove the logic of handling rxReasonToCPUQueue as we already deprecated this field. Let me know once you remove the relevant code for this field.
Besides, when reviewing this PR, it made me think we might need 3 more improvements related to copp and you can address them in the future PRs respectively:
- Rename cpu_queue to just queue. It's already under copp, so queue implicitly mean cpu_queue, so we don't need to be verbose about that;
- About your comment in the summary that
config copp cpu-queuecan only support changing name and rate_limit. FBOSS supports cpu queue just as port queue, can you make another PR to make sure config for cpu queue with the missing attributes? - Since
rxReasonToQueueOrderedListis actually an ordered list, I think we need to adjust the currentconfig copp reason <reason-name> queue <id>to allow specify order rather than always push_back to the list:
https://github.com/facebook/fboss/blob/main/fboss/cli/fboss2/commands/config/copp/CmdConfigCopp.cpp#L276
cc: @benoit-nexthop
|
|
||
| namespace facebook::fboss { | ||
|
|
||
| namespace copp_cpu_queue { |
There was a problem hiding this comment.
Can you create another PR to simplify the subcommand here to just be copp_queue or replace all the cpu_queue with just queue?
copp already means control plane so the queue used in here already means cpu queue, I think we can make the whole command less verbose.
I think these should work:
fboss2-dev config copp queue XXX
fboss2-dev delete copp queue XXX
| // The deprecated rxReasonToCPUQueue map is still honored by the agent as | ||
| // a fallback when the ordered list is unset, so a queue referenced only | ||
| // there is still live. | ||
| if (policy.rxReasonToCPUQueue().has_value()) { | ||
| for (const auto& [rxReason, mappedQueueId] : *policy.rxReasonToCPUQueue()) { | ||
| if (mappedQueueId == queueId) { | ||
| references.push_back( | ||
| fmt::format( | ||
| "reason {} (deprecated rxReasonToCPUQueue map)", | ||
| apache::thrift::util::enumNameSafe(rxReason))); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
You can ignore this field as we don't use it internally anymore.
Besides we don't support setting the reason to the deprecated field in
https://github.com/facebook/fboss/blob/main/fboss/cli/fboss2/commands/config/copp/CmdConfigCopp.cpp#L253
I think we should just stop using rxReasonToCPUQueue at all
| // The deprecated rxReasonToCPUQueue map form is still read by the agent as | ||
| // a fallback when rxReasonToQueueOrderedList is unset, so a queue referenced | ||
| // only there must also refuse deletion. The CLI never writes this map; it | ||
| // only appears in configs authored outside the CLI. |
There was a problem hiding this comment.
Let's stop supporting this deprecated field: rxReasonToCPUQueue
The field is deprecated and no longer honored internally, so a queue referenced only by the legacy map is not live; stop treating it as a blocking reference and drop the legacy-map fixture.
|
@vybhav-nexthop has updated the pull request. You must reimport the pull request before landing. |
Pre-submission checklist
pip install -r requirements-dev.txt && pre-commit installpre-commit runSummary
What: Delete-side counterparts for the
coppconfig commands:delete copp cpu-queue <id>— removes the wholesw.cpuQueues[]entry. Refused while rxReason mappings (rxReasonToQueueOrderedList) or cpu-traffic-policy actions (send-to-queue, user-defined-trap) still reference the queue id; the error names each referencing entry.delete copp reason <reason-name>— removes therxReasonToQueueOrderedListentry.Why: The config-side copp commands landed without a way to unset what they set.
Notes
Whole-queue delete is not undoable by the CLI
delete copp cpu-queue <id>erases the entirecpuQueues[]entry, matchingdelete qos default-queue-config <id>, which erases a whole entry fromdefaultPortQueues— samelist<PortQueue>shape.The difference is on the config side.
config qos default-queue-configcan writereserved-bytes,shared-bytes,weight,scaling-factor,scheduling,stream-type,buffer-pool-nameand AQM entries viautils::applyPortQueueConfig, so a deleted default queue can be rebuilt by hand.config copp cpu-queuewrites onlynameandportQueueRate, so a deleted cpu-queue cannot.PortQueuehas 17 fields and the CLI can set two of them (name, andportQueueRateviarate-limit).streamTypeandschedulingare non-optional in thrift, so a recreated queue silently picks up their zero values. A platform agent.conf ships queue 9 as:{ "id": 9, "name": "cpuQueue-high", "scheduling": 1, "streamType": 1 }scheduling1 isSTRICT_PRIORITY,streamType1 isMULTICAST. Delete it and recreate it and you getscheduling0 (WEIGHTED_ROUND_ROBIN) andstreamType0 (UNICAST), with no command to set either back. The high-priority CPU queue would start round-robining.So: delete a platform-provisioned cpu-queue only when you want it gone for good. Queues you created yourself are safe to delete and recreate.
To change a name or rate limit, no delete is involved — config overwrites in place:
delete copp reasontakes no queue idConfig is
config copp reason <reason> queue <id>, delete isdelete copp reason <reason>.rxReasonToQueueOrderedListholds at most one entry per reason — config overwrites an existing reason rather than appending a second entry — so the reason name alone identifies the entry, and delete looks it up that way:Passing a queue id is rejected rather than ignored (
->expected(1), plus the arg parser throws on a second token). The queue id shows up on the way out instead —Deleted reason ARP -> queue 9 mapping— read off the entry being removed.Re-pointing a reason at a different queue is a config, not a delete:
config copp reason arp queue 5printsUpdated reason ARP -> queue 5. Delete is for taking the reason off the CPU path entirely.Clearing what blocks a queue delete
The refusal message lists each referrer:
reason <NAME>for rxReason mappings,matcher '<name>' send-to-queue/matcher '<name>' user-defined-trapfor cpu-traffic-policy actions (findQueueReferences(),CmdDeleteCoppCpuQueue.cpp). Only those two action types carry a queue id, so only they can block.Clear reasons with
delete copp reason <reason>; clearing a blocking action needs the cpu-traffic-policy action delete from #1379:The deprecated
rxReasonToCPUQueuemap is ignored entirely (per review — the field is no longer used).Test Plan
Unit tests — 13 added by this PR (7 in
CmdDeleteCoppCpuQueueTestFixture, 6 across theCmdDeleteCoppReason*fixtures), all passing:Integration tests on a freshly imaged T1 device — 2/2 pass)
One end-to-end test per command, both unconditional .
Each test creates what it deletes (
config copp reasoncreatescpuTrafficPolicyandrxReasonToQueueOrderedListwhen absent) and removes it again, so nothing touches a live CoPP policy.Sample usage on the same device — note the refusal list shrinks as the referencing reason is deleted:
Review Findings
Pre-publication review (7 reviewers + verifier, confidence >= 0.7) surfaced these findings; all are addressed in this PR:
CoppUtils.{h,cpp}instead of living in the config command headeruser-defined-traprefusal branch gained a unit testcfg::PacketRxReasonenum values instead of bare ints